Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Migrate SCSS $clickable-area to CSS --default-clickable-area #5694

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

marcoambrosini
Copy link
Contributor

src/assets/action.scss Outdated Show resolved Hide resolved
src/assets/variables.scss Outdated Show resolved Hide resolved
susnux

This comment was marked as resolved.

@marcoambrosini marcoambrosini force-pushed the feat/change-default-clickable-area branch from 250e77d to 193f916 Compare June 17, 2024 09:35
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar with the other cases

src/assets/NcAppNavigationItem.scss Outdated Show resolved Hide resolved
src/assets/NcAppNavigationItem.scss Outdated Show resolved Hide resolved
src/assets/variables.scss Outdated Show resolved Hide resolved
src/components/NcActionInput/NcActionInput.vue Outdated Show resolved Hide resolved
@marcoambrosini marcoambrosini force-pushed the feat/change-default-clickable-area branch from 193f916 to cfcf998 Compare June 18, 2024 08:06
@marcoambrosini marcoambrosini force-pushed the feat/change-default-clickable-area branch from cfcf998 to 4557fb5 Compare June 18, 2024 08:09
@susnux susnux force-pushed the feat/change-default-clickable-area branch 3 times, most recently from b325efc to 929cf08 Compare June 20, 2024 16:05
@susnux susnux requested a review from ShGKme June 20, 2024 16:06
@susnux susnux added bug Something isn't working 3. to review Waiting for reviews design Design, UX, interface and interaction design labels Jun 20, 2024
@susnux susnux force-pushed the feat/change-default-clickable-area branch 2 times, most recently from c9b32f6 to fe89ea7 Compare June 20, 2024 16:24
susnux

This comment was marked as resolved.

@susnux susnux mentioned this pull request Jun 20, 2024
1 task
src/assets/variables.scss Outdated Show resolved Hide resolved
src/components/NcInputField/NcInputField.vue Outdated Show resolved Hide resolved
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
src/components/NcColorPicker/NcColorPicker.vue Outdated Show resolved Hide resolved
@marcoambrosini marcoambrosini force-pushed the feat/change-default-clickable-area branch from fe89ea7 to ee87644 Compare June 21, 2024 08:04
@susnux susnux dismissed their stale review June 21, 2024 09:17

Changes were overwritten, need to re-review

@susnux susnux self-requested a review June 21, 2024 09:17
@marcoambrosini marcoambrosini force-pushed the feat/change-default-clickable-area branch from ee87644 to 0272cd4 Compare June 29, 2024 06:35
@marcoambrosini
Copy link
Contributor Author

@susnux @ShGKme would it be ok like this?

@marcoambrosini
Copy link
Contributor Author

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unbreaking for 28 and 29, for 30 it fixes some weird places so 👍

@@ -540,7 +540,7 @@ $input-margin: 4px;
min-height: 0;
/* Keep padding to define the width to
assure correct position of a possible text */
padding: #{math.div($clickable-area, 2)} 0 #{math.div($clickable-area, 2)} $clickable-area;
padding: calc(var(--default-clickable-area)/ 2) 0 calc(var(--default-clickable-area)/ 2) var(--default-clickable-area);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should cache calc(var(--default-clickable-area)/ 2)? (note for the future)

@@ -215,15 +215,15 @@ $input-margin: 4px;
min-height: 0;
/* Keep padding to define the width to
assure correct position of a possible text */
padding: #{math.div($clickable-area, 2)} 0 #{math.div($clickable-area, 2)} $clickable-area;
padding: calc(var(--default-clickable-area) / 2) 0 calc(var(--default-clickable-area) / 2) var(--default-clickable-area);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because here we use it again

@@ -258,10 +258,10 @@ $input-margin: 4px;

// bottom-right corner
position: absolute;
right: $icon-margin + 1;
right: calc($icon-margin + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should get rid of this options in the future (...-right, ...-left, right, left) to properly support RTL layout and just use ...-inline-start ...-inline-end.

Just a note for the future, not related to this PR 😅

@susnux susnux changed the title Feat/change default clickable area fix: Migrate SCSS $clickable-area to CSS --default-clickable-area Jul 1, 2024
@susnux susnux force-pushed the feat/change-default-clickable-area branch from a4084be to 8df3711 Compare July 1, 2024 10:44
@susnux
Copy link
Contributor

susnux commented Jul 1, 2024

Todo:

  • Update snapshots (I am pretty sure Cypress will fail otherwise)...

marcoambrosini and others added 2 commits July 1, 2024 12:46
Co-authored-by: Ferdinand Thiessen <[email protected]>
Co-authored-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Marco <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the feat/change-default-clickable-area branch from 8df3711 to 5f2ff37 Compare July 1, 2024 10:52
@susnux susnux merged commit 01500a9 into master Jul 1, 2024
19 checks passed
@susnux susnux deleted the feat/change-default-clickable-area branch July 1, 2024 10:54
@susnux
Copy link
Contributor

susnux commented Jul 1, 2024

/backport to next

@nickvergessen
Copy link
Contributor

Milestone is 8.14 for this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Design, UX, interface and interaction design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants